refactor: new icons using icon gallery #4932
Conversation
fbwoolf
requested review from
kyranjamie,
pete-watters,
edgarkhanzadian and
fabric-8
and removed request for
edgarkhanzadian
fbwoolf
force-pushed
the
refactor/icons
branch
from
b798688 to
c9655e1
Compare
|
@fbwoolf I accidentally deleted your comment. So sorry! Can't seem to retrieve it. But the "Current implementation with mdx docs and one example icon story" is perfect! The only thing I noticed so far is the naming "Standard 16x16" and "Standard 24x24", while it should be "Small 16x16" and "Default 24x24" I also dont think we need one story per icon as this would serve the modulation of the icons, however this is already defined by us when bringing the icon into Figma and defining it to be 16 and 24. Great job!!! 🥳 |
fbwoolf
force-pushed
the
refactor/icons
branch
3 times, most recently
from
ad0f1c2 to
7eb6805
Compare
|
@fbwoolf Happy to see this happen and the the storybook previewing is super useful! Do you already want me to check out the build or should I hold back a little longer? Would also be great if we could switch out the home icon for the new ones while at it :) 
|
Nice, yeah I can swap those out. I am still testing the changes bc broke some things but will open the PR tonight or tom and will ping you to test once open. Thanks for the feedback today in Figma. |
fbwoolf
force-pushed
the
refactor/icons
branch
2 times, most recently
from
53f2f52 to
b1389df
Compare
|
|
I need to fix the lint issue here, but opening it up for review and design QA. |
src/app/ui/icons/icon.tsx
Outdated
| color?: string; | ||
| opacity?: string; | ||
| strokeWidth?: string; | ||
| width?: string; |
There was a problem hiding this comment.
Saw you replied to comment, but these are still here? Are they needed for some reason?
There was a problem hiding this comment.
There is a comment in the code left as to why. I removed them, but got type errors bc the underlying component we use is a path element not a styled component which seem to create type errors unless I redefined here. I can try using a styled.path and removing them but not sure if that will work and did not seem to allow for changing the color with the color picker in storybook (which maybe isn't nec?).
There was a problem hiding this comment.
I figured out a better way to handle the types, so I was able to remove these again.
There was a problem hiding this comment.
I think it could be good to keep the mdx right beside the story so thing.stories.mdx isn't a bad pattern.
I'm fine with a docs/ folder too though
src/app/ui/icons/icons.mdx
Outdated
| @@ -0,0 +1,50 @@ | |||
| import { IconGallery, IconItem, Meta } from '@storybook/blocks'; | |||
There was a problem hiding this comment.
As this is exclusive for storybook, maybe the filename should annotate this? Is there a common pattern for this?
There was a problem hiding this comment.
There are two patterns I was ref'ing from storybook examples, there are links in the description here. There wasn't a pattern in naming with mdx files that I saw. Typically it was just named as the component name with the extension or something like introduction.mdx for a doc file introducing the example.
There was a problem hiding this comment.
I created a docs folder and moved it there.
src/app/ui/icons/icons-list.ts
Outdated
| @@ -0,0 +1,53 @@ | |||
| export const iconsList = [ | |||
There was a problem hiding this comment.
If this is only for storybook, isn't a node function to get list from tree trivial and not require updating?
There was a problem hiding this comment.
I can look at this, I followed what the storybook example did, but that might be a better so can give it a shot.
There was a problem hiding this comment.
After looking at this, I'm not sure it is trivial to do and we might want to control the gallery shown manually? I did reorg the files a bit so there is a docs folder which contains the mdx file and the list to display in the docs gallery.
There was a problem hiding this comment.
Nvm, this is pretty trivial so changed.
fbwoolf
force-pushed
the
refactor/icons
branch
5 times, most recently
from
a70ebc8 to
09c994f
Compare
fbwoolf
force-pushed
the
refactor/icons
branch
from
09c994f to
dc7efa9
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@fabric-8 I merged this into dev so @pete-watters can use the icons in his new settings work. If you test it and find any needed changes, can you track them in a new issue and assign to me? |
|
I just noticed that the Collectibles loading spinner looks different. Is that intentional? LoadingIcon.mp4 |
I did replace it with a new icon called EDIT: O, I see now you meaning the animation! Haha, I will look at it and fix! Thx. |
EDIT: OPEN FOR REVIEW
Note: there ended up being a constant issue trying to pass the prop
sizewith panda and assigning it to the svg height and width, so I ended up changing the prop name on all the icons to just be setting height and width using the size token instead.Draft proposal:
I wanted to open this up as a draft for feedback on the direction. I ended up following the
storybookjs/iconspattern so that we can have anmdxdocs file that shows all the icons on one page, while at the same time we have the flexibility to add a story for each icon if we feel it is necessary. I prefer how the first example, the one I followed, ends up rendering in Storybook even though the code is a bit less DRY.Example I followed:
https://github.com/storybookjs/icons
(storybook)
https://main--64b56e737c0aeefed9d5e675.chromatic.com/?path=/docs/introduction--docs
This is another example we could follow where we simplify the amount of icon components we have by instead keeping a shared list of just icon
pathsand render them in one icon story:Alternative example:
https://github.com/storybookjs/design-system/blob/b852133e5e75f410a4f43171ceb31438e344b5b3/src/components/Icon.tsx
(storybook)
https://master--5ccbc373887ca40020446347.chromatic.com/?path=/docs/icon--docs
Current implementation with mdx docs and one example icon story:
